feat(review-agent): add two-pass review strategy for large PRs#190
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://d6666021-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 Finished Review · ✅ Success · Started 8:10 PM UTC · Completed 8:24 PM UTC |
ReviewFindingsMedium
Labels: PR modifies review agent skill definitions and adds a new sub-agent under skills/pr-review/. Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsLow
Info
Previous run (4)ReviewFindingsMedium
Low
Info
Labels: PR modifies review agent skill definitions and sub-agent roster, which is harness/skills infrastructure. Previous run (5)ReviewFindingsMedium
Low
Info
|
🤖 Review · ❌ Terminated · Started 6:57 PM UTC · Ended 7:10 PM UTC |
/fs-fix but ignore
|
🤖 Finished Fix · ✅ Success · Started 7:03 PM UTC · Completed 7:11 PM UTC |
🤖 Finished Review · ✅ Success · Started 6:57 PM UTC · Completed 7:10 PM UTC |
🔧 Fix agent — iteration 1 (human-triggered)Addressed 4 of 8 non-excluded review findings (1 medium, 3 low). Fixed threshold mismatch by raising triage trigger from 30 to 50 files to align with step 2's per-file diff boundary. Added clarifying notes for non-standard dispatch types in roster table and step 4. Removed parenthetical from sub-agent title. Disagreed with 4 findings that were already documented, followed existing convention, or were out of scope. Per human instruction, ignored [architectural-conflict], [fail-open], and info-level findings. Fixed (4):
Disagreed (4):
Tests: passed Updated by fullsend fix agent |
🤖 Finished Review · ✅ Success · Started 7:15 PM UTC · Completed 7:28 PM UTC |
🤖 Finished Review · ✅ Success · Started 7:39 PM UTC · Completed 7:53 PM UTC |
/fs-fix Address these Low review findings
|
|
🤖 Finished Review · ✅ Success · Started 8:56 PM UTC · Completed 9:15 PM UTC |
🤖 Finished Fix · ✅ Success · Started 8:02 PM UTC · Completed 8:14 PM UTC |
🔧 Fix agent — iteration 2 (human-triggered)Addressed all 4 low-severity review findings (2 fail-open, 1 api-shape, 1 doc-style). Hardened triage classification with larger diff windows for non-path-matched files, empty-result validation against path patterns, explicit raw JSON output constraint, and corrected heading level. Fixed (4):
Tests: passed Updated by fullsend fix agent |
🤖 Finished Review · ✅ Success · Started 8:18 PM UTC · Completed 8:35 PM UTC |
/fs-fix Address [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The triage sub-agent's path pattern list does not include several security-sensitive paths from the orchestrator's protected-path list (e.g., .github/ beyond workflows, .claude/, root-level policies/). A file under .claude/ could be classified as "standard" by triage, receiving deprioritized context. Mitigated by step 6e's protected-path check running independently in the orchestrator regardless of triage classification. and also make sure that all of the security-sensitive paths in the orchestrator, subagents, and related skills are guarded again recursion (i.e. paths below e.g. ".claude/" must also be protected). |
🤖 Finished Fix · ✅ Success · Started 7:08 PM UTC · Completed 7:16 PM UTC |
🔧 Fix agent — iteration 3 (human-triggered)Aligned security-triage sub-agent path patterns with the orchestrator's full protected-path list. Added 16 governance/infrastructure paths as a new classification category with recursive glob patterns. Updated orchestrator fallback validation examples to reference the new patterns. Fixed (3):
Tests: passed Updated by fullsend fix agent |
🤖 Finished Review · ❌ Failure · Started 5:42 PM UTC · Completed 5:49 PM UTC |
/fs-review |
🤖 Finished Review · ✅ Success · Started 7:25 PM UTC · Completed 7:37 PM UTC |
PR Summary by QodoAdd security-triage pre-pass to prioritize security review in large PRs
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
/fs-fix - The new triage step is gated solely on FILE_COUNT ≥ 50, but the earlier “large PR → per-file mode” criteria is documented as a combined FILE_COUNT+LINE_COUNT decision. The triage rationale and prompt composition assume per-file diffs/summaries are available, which is not guaranteed by the current gating - Step 3c-1’s validation logic is underspecified: it doesn’t require that the union of |
|
🤖 Finished Fix · ✅ Success · Started 2:26 PM UTC · Completed 2:33 PM UTC |
|
🤖 Finished Review · ✅ Success · Started 2:15 PM UTC · Completed 2:31 PM UTC |
🔧 Fix agent — iteration 5 (human-triggered)Aligned step 3c-1 triage gating with step 2's per-file mode decision (combined FILE_COUNT+LINE_COUNT) instead of FILE_COUNT alone, and strengthened validation with completeness/dedup checks and deterministic path-pattern override enforcement. Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 2:35 PM UTC · Completed 2:43 PM UTC |
|
@ben-alkov feel free to merge this |
b849563 to
14ce392
Compare
|
🤖 Review · |
|
🤖 Review · |
For PRs with 30+ files, the review orchestrator now runs a lightweight security-triage pre-pass before dispatching dimension sub-agents. The triage pass uses a haiku-model sub-agent to classify changed files as security-critical or standard based on path patterns (e.g., **/mint/**, **/auth/**, **/oidc/**) and diff content heuristics (auth logic, token handling, permission changes). Security-critical files identified by the triage pass receive prioritized context in the security and correctness sub-agent context packages — their full diffs appear first with explicit classification headers, ensuring they get dedicated reasoning budget rather than competing with boilerplate changes. Changes: - New sub-agent definition: sub-agents/security-triage.md (haiku model, read-only classifier) - New orchestrator step 3c-1 in SKILL.md: security-critical file triage, runs synchronously before context package assembly - Updated step 3d in SKILL.md: security-prioritized context package assembly for security and correctness sub-agents when triage results are available - Updated sub-agent roster table with security-triage entry The 30-file threshold is a starting point that may need tuning. Triage failures fall back to uniform attention (all files treated as security-critical) to preserve existing behavior as a safe default. Closes #2096 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
- Raise security triage threshold from 30 to 50 files to align with step 2's per-file diff boundary, resolving ambiguity in the 30-49 file range where per-file diffs were not available [edge-case] - Add clarifying note to roster table documenting that security-triage and challenger use non-standard dispatch and are excluded from step 4's parallel loop [logic-error, design-direction] - Clarify step 4 heading to explicitly scope dispatch to dimension sub-agents only [logic-error] - Remove parenthetical from security-triage sub-agent title to match naming convention of other sub-agents [naming-convention] Addresses review feedback on #2303 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
- [fail-open] Increase diff summary window from ~20 to ~50 lines for files that don't match security path patterns, giving the classifier more content signal for non-obvious security-relevant changes - [fail-open] Treat empty security_critical_files as triage failure when changed files match known security-sensitive path patterns - [api-shape] Add explicit constraint requiring raw JSON output (no markdown code fences) to security-triage sub-agent - [doc-style] Convert bold-with-colon header to proper #### heading for the security-prioritized context subsection Addresses review feedback on #2303 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
… protected paths Add governance and infrastructure paths (`.claude/`, `.github/`, `agents/`, `scripts/`, `skills/`, etc.) to the security-triage sub-agent's classification criteria. These paths were present in the orchestrator's step 6e protected-path list but missing from the triage classifier, which could cause files under those paths to be classified as "standard" and receive deprioritized context in large PRs. All directory paths use `/**` glob suffix to ensure recursive matching of files below the directory root. The narrower `.github/workflows/*.yml` pattern is replaced by `.github/**` to cover all GitHub configuration files. The orchestrator's fallback validation examples in step 3c-1 are updated to reference the new governance patterns. Addresses review feedback on #2303 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
…ring - Update diff summary format block in step 3c-1 to reflect the two-tier excerpt system (~20 lines for path-pattern matches, ~50 lines otherwise) - Number the Security-prioritized context section as step 3f for consistency with the other numbered sub-sections under step 3 Signed-off-by: Ben Alkov <ben.alkov@redhat.com>
…engthen validation Gate step 3c-1 security triage on step 2's per-file mode decision (combined FILE_COUNT + LINE_COUNT thresholds) instead of FILE_COUNT alone — the triage prompt requires per-file diffs that are only produced in per-file mode. Add structural validation to the triage classification result: completeness (union of classified files must equal changed-file set), no-duplicates (no path in both arrays or repeated), and path-pattern override (files matching classification path patterns are authoritatively moved to security-critical regardless of classifier output). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b75228a to
12f47e4
Compare
|
🤖 Finished Review · ✅ Success · Started 6:36 PM UTC · Completed 7:18 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 6:56 PM UTC · Completed 7:18 PM UTC |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
Retro: PR #190 — Two-Pass Review Strategy for Large PRsWorkflow OverviewPR #190 added a security-triage pre-pass to the review orchestrator for PRs with 50+ files. A haiku-model sub-agent classifies changed files as security-critical or standard, then security/correctness sub-agents receive prioritized context. The lifecycle spanned 44 days (Jun 9 issue → Jul 23 merge) with 16 review runs, 6 fix iterations, and 67 total workflow runs across the fullsend and agents repos. The PR originated as fullsend#2303 (code agent, Jun 15), went through 10 review/fix cycles there, was cloned to agents#190 on Jul 15 when the skill files settled in the agents repo, then went through 5 more review runs and 2 more fix iterations before merge. Review QualityThe review agent performed well. It correctly identified a frontmatter naming convention violation ( Autonomy Observationrh-hemartin approved with "LGTM" on Jul 16, before the review agent's Jul 17 findings about the naming convention and logic gap. The review agent caught convention and cross-reference issues that the human reviewer did not flag, suggesting the agent adds value for structural and convention-level analysis beyond typical human review. Rework AnalysisThe 6 fix iterations addressed: wrong threshold (30→50), inconsistent trigger condition, misaligned path patterns, missing structural validation, gating issues, and wrong frontmatter name. Most could have been avoided with better initial output from the code agent — particularly studying existing thresholds in the SKILL.md and sibling sub-agent frontmatter conventions. Evidence for Existing Issues
Proposals filed
|
For PRs with 50+ files, the review orchestrator now runs a lightweight security-triage pre-pass before dispatching dimension sub-agents. The triage pass uses a haiku-model sub-agent to classify changed files as security-critical or standard based on path patterns (e.g., /mint/, /auth/, /oidc/) and diff content heuristics (auth logic, token handling, permission changes).
Security-critical files identified by the triage pass receive prioritized context in the security and correctness sub-agent context packages — their full diffs appear first with explicit classification headers, ensuring they get dedicated reasoning budget rather than competing with boilerplate changes.
Changes:
model, read-only classifier)
triage, runs synchronously before context package assembly
assembly for security and correctness sub-agents when triage
results are available
The 50-file threshold is a starting point that may need tuning. Triage failures fall back to uniform attention (all files treated as security-critical) to preserve existing behavior as a safe default.
Closes fullsend-ai/fullsend#2096
Post-script verification
agent/2096-two-pass-review-strategy)f119a205ffb44da8687d9ff1673b6b2d2de2e927..HEAD)Original PR metadata